Font Library: sanitize font collection data #58636

matiasbenedetto · 2024-02-02T21:07:42Z

What?

Font Library: sanitize font collection data.

Removes all the potential unwanted properties.
Sanitizes all the values.
Adds unit test case to ensure that data returned by get_data is always sanitized.

Why?

To return just the data that's safe to return.

How?

Adding a data schema in the WP_Font_Collection class.
Sanitizing the data using the sanitizing util from WP_Font_Utils

Testing Instructions

Run this PHP snippet featuring a font collection with risky data:

function register_collections() {
    $ubuntu = array(
        'font_family_settings' => array (
            'name'       => 'Ubuntu<script>alert("xss")</script>',
            'fontFamily' => 'Ubuntu, sans-serif<script>alert("xss")</script>',
            'slug'       => 'ubuntu<script>alert("xss")</script>',
        ),
        'categories' => array(
            'sans-serif',
        ),
    );
    
    $verdana = array(
        'font_family_settings' => array (
            'name'       => 'Verdana<script>alert("xss")</script>',
            'fontFamily' => 'Verdana, sans-serif<script>alert("xss")</script>',
            'slug'       => 'verdana<script>alert("xss")</script>',
        ),
        'categories' => array(
            'sans-serif',
        ),
    );
    
    $font_families = array ( $ubuntu, $verdana );
    
    $categories = array(
        array (
            'name' => 'Sans Serif<script>alert("xss")</script>',
            'slug' => 'sans-serif<script>alert("xss")</script>',
        ),
    );
    
    $collection_with_xss = array(
        'name'          => 'PHP Custom Collection',
        'description'   => __( 'Custom fonts collection' ),
        'font_families' => $font_families,
        'categories'    => $categories
    );
    
    wp_register_font_collection( 'collection-with-xss', $collection_with_xss );
}

add_action( 'rest_api_init', 'register_collections' );

Request that font collection using the API:

/wp-json/wp/v2/font-collections/collection-with-xss

the response should be:

{
    "slug": "collection-with-xss",
    "name": "PHP Custom Collection",
    "description": "Custom fonts collection",
    "font_families": [
        {
            "font_family_settings": {
                "name": "Ubuntu",
                "fontFamily": "Ubuntu, sans-serif",
                "slug": "ubuntualertxss"
            },
            "categories": [
                "sans-serif"
            ]
        },
        {
            "font_family_settings": {
                "name": "Verdana",
                "fontFamily": "Verdana, sans-serif",
                "slug": "verdanaalertxss"
            },
            "categories": [
                "sans-serif"
            ]
        }
    ],
    "categories": [
        {
            "name": "Sans Serif",
            "slug": "sans-serifalertxss"
        }
    ],
    "_links": {
        "self": [
            {
                "href": "http://localhost/wp1/wp-json/wp/v2/font-collections/collection-with-xss"
            }
        ],
        "collection": [
            {
                "href": "http://localhost/wp1/wp-json/wp/v2/font-collections"
            }
        ]
    }
}

github-actions · 2024-02-02T21:07:57Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core SVN

If you're a Core Committer, use this list when committing to wordpress-develop in SVN:

Props: mmaattiiaass, grantmkin, youknowriad.

GitHub Merge commits

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org>
Co-authored-by: creativecoder <grantmkin@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-02-02T21:08:50Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php
❔ lib/compat/wordpress-6.5/fonts/class-wp-font-utils.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/__construct.php
❔ phpunit/tests/fonts/font-library/wpFontCollection/getData.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/unregisterFontCollection.php

lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php

…ys sanitized following the font collection schema

- Sanitize and validate as early as possible, so that appropriate notices are logged right away - Use static method for sanitization schema so that we can use a closure rather than a public method for src sanitization - Adds a check for WP_Font_Utils::sanitize_from_schema so that class callables like array( $this, 'sanitization_method' ) can be used - Updates method name to indicate that sanitization is done first, as this might remove invalid data and affect the validation result

creativecoder · 2024-02-03T01:37:18Z

@matiasbenedetto I made an attempt to streamline this and pushed a commit to this branch:

Sanitize and validate as early as possible, so that appropriate notices are logged right away.
Use static method for sanitization schema rather than a class constant so that we can use a closure for src sanitization rather than needing to add a new public method to the class.
Adds a check within WP_Font_Utils::sanitize_from_schema so that class callables like array( $this, 'sanitization_method' ) can also be used
Updates method name to sanitize_and_validate_data to indicate that sanitization is done first, as it might remove invalid data and affect the validation result.

Feel free to revert if you don't agree with the changes. Otherwise I think this now looks ready to go.

creativecoder · 2024-02-03T01:30:31Z

lib/compat/wordpress-6.5/fonts/class-wp-font-utils.php

@@ -158,7 +158,7 @@ public static function sanitize_from_schema( $tree, $schema ) {
 				}

 				$is_value_array  = is_array( $value );
-				$is_schema_array = is_array( $schema[ $key ] );
+				$is_schema_array = is_array( $schema[ $key ] ) && ! is_callable( $schema[ $key ] );


Adding this check allows using a class instance method, like array( $this, 'sanitization_method' ).

matiasbenedetto requested a review from spacedmonkey as a code owner February 2, 2024 21:07

matiasbenedetto added [Type] Security Related to security concerns or efforts [Feature] Font Library labels Feb 2, 2024

matiasbenedetto requested review from pbking and creativecoder February 2, 2024 21:08

creativecoder reviewed Feb 2, 2024

View reviewed changes

lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php Outdated Show resolved Hide resolved

lib/compat/wordpress-6.5/fonts/class-wp-font-collection.php Outdated Show resolved Hide resolved

matiasbenedetto added 7 commits February 2, 2024 19:26

sanitize font collection data

6becc25

add a test case to be sure that the data returned by get_data is alwa…

148bcb0

…ys sanitized following the font collection schema

format php

ccdf2f9

lint

9e52287

custom function to validate src properties

6b88f9b

rename function

98e8295

php format

cd5b338

creativecoder force-pushed the add/sanitize-font-collection-data branch from 95389bf to a581183 Compare February 3, 2024 01:26

creativecoder force-pushed the add/sanitize-font-collection-data branch from a581183 to f307972 Compare February 3, 2024 01:27

creativecoder reviewed Feb 3, 2024

View reviewed changes

Fix potential nitpicks

37e899a

creativecoder mentioned this pull request Feb 5, 2024

Font Library: WordPress 6.5 Required Tasks #55277

Closed

25 tasks

youknowriad approved these changes Feb 5, 2024

View reviewed changes

youknowriad merged commit b942f10 into trunk Feb 5, 2024
57 checks passed

youknowriad deleted the add/sanitize-font-collection-data branch February 5, 2024 08:39

github-actions bot added this to the Gutenberg 17.7 milestone Feb 5, 2024

creativecoder mentioned this pull request Feb 6, 2024

Font Library: Ensure setting values are sanitized #58464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Font Library: sanitize font collection data #58636

Font Library: sanitize font collection data #58636

matiasbenedetto commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

creativecoder commented Feb 3, 2024

creativecoder Feb 3, 2024

Font Library: sanitize font collection data #58636

Font Library: sanitize font collection data #58636

Conversation

matiasbenedetto commented Feb 2, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Feb 2, 2024 • edited Loading

Core SVN

GitHub Merge commits

github-actions bot commented Feb 2, 2024 • edited Loading

creativecoder commented Feb 3, 2024

creativecoder Feb 3, 2024

Choose a reason for hiding this comment

matiasbenedetto commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading